Review status: 0 of 10 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

ycmd/completers/completer.py, line 197 [r1] (raw file):
I understand this guarantees us that a completer is well implemented but I would rather keep it as a default implementation, hence a completer which do not have any subcommands would not need to implement a dummy method like we have to do right now for the filename_completer the general_completer and the general_completer_store. If you think about it a very important method like ComputeCandidatesInner is not an abstract method.

ycmd/completers/completer.py, line 202 [r1] (raw file):
What do you think about a map of command to lambda functions? So instead of porting all the completer to the clang_completer format we do the opposite (IIRC is also how @puremourning prototype the tern.js completer).

Comments from the review on Reviewable.io

Review status: 0 of 10 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

ycmd/completers/completer.py, line 197 [r1] (raw file):
I'll remove it in this PR but I think we need a new subclass for semantic completers (SemanticCompleter for example) and abstract the SupportedFiletypes and GetSubcommandsMap methods in it. Those are specific to semantic completers.

I also think that FilenameCompleter should derive from GeneralCompleter like UltisnipsCompleter and IdentifierCompleter. GeneralCompleterStore should not be a completer. This is just a container.

But this will be for another PR.

ycmd/completers/completer.py, line 202 [r1] (raw file):
I need to try it. See if it is simpler.

Comments from the review on Reviewable.io

Review status: 0 of 10 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

ycmd/completers/completer.py, line 202 [r1] (raw file):
Yeah, i'm responsible for the crazy dictionary approach from clang_completer, and i'm not super proud of it. I think the map of lamda is more flexible, and probably neater.

For tern completer, I did something similar to cs completer:

  subcommands = {
    'StartServer':     ( lambda self, request_data, args:
                                        self._StartServer() ),
    'StopServer':      ( lambda self, request_data, args:
                                        self._StopServer() ),
    'ConnectToServer': ( lambda self, request_data, args:
                                       self._ConnectToServer( args ) ),
    'GoToDefinition':  ( lambda self, request_data, args:
                                        self._GoToDefinition( request_data) ),
    'GoTo':            ( lambda self, request_data, args:
                                        self._GoToDefinition( request_data) ),
    'GetType':         ( lambda self, request_data, args:
                                        self._GetType( request_data) ),
    'GetDoc':          ( lambda self, request_data, args:
                                       self._GetDoc( request_data) ),
  }

Called like this:

  def DefinedSubcommands( self ):
    return self.subcommands.keys()

  def OnUserCommand( self, arguments, request_data ):
    if not arguments or arguments[ 0 ] not in TernCompleter.subcommands:
      raise ValueError( self.UserCommandsHelpMessage() )

    return TernCompleter.subcommands[ arguments[ 0 ] ]( self,
                                                        request_data,
                                                        arguments[ 1: ] )

Comments from the review on Reviewable.io

Updates:

• GetSubcommandsMap is not an abstract method anymore;
• Dictionaries are replaced by lambda functions in subcommands map.

Review status: 0 of 7 files reviewed at latest revision, 2 unresolved discussions.

ycmd/completers/completer.py, line 202 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

Fix also a typo in the Clang completer.

Comments from the review on Reviewable.io

Reviewed 1 of 10 files at r1, 5 of 9 files at r2.
Review status: 6 of 7 files reviewed at latest revision, 3 unresolved discussions.

ycmd/completers/completer.py, line 268 [r2] (raw file):
So we use only the first argument of arguments? Don't we support subcommands that take arguments?

Comments from the review on Reviewable.io

Review status: 6 of 7 files reviewed at latest revision, 3 unresolved discussions.

ycmd/completers/completer.py, line 268 [r2] (raw file):
Yeah I think we should pass the remaining args, maybe via *args[1:] ?

Fwiw I need to pass an arg for tern, so maybe I can make that change on top (as for now it is yagni)

Comments from the review on Reviewable.io

ycmd/completers/completer.py, line 268 [r2] (raw file):
Yes, I forgot that. Let me change it.

Comments from the review on Reviewable.io

Review status: 6 of 7 files reviewed at latest revision, 3 unresolved discussions.

ycmd/completers/completer.py, line 268 [r2] (raw file):
If right now is not needed then we could leave it like this and add support later which would be covered by a test I imagine.

Comments from the review on Reviewable.io

ycmd/completers/completer.py, line 268 [r2] (raw file):
Ok, not implemented in this PR then. For reference, adding arguments[ 1: ] to this line and a parameter to the lambda functions is enough.

Comments from the review on Reviewable.io

Reviewed 1 of 9 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

ycmd/completers/completer.py, line 197 [r1] (raw file):

GeneralCompleterStore should not be a completer. This is just a container.

I don't agree with this, it is not just a container but basically a Facade which merges candidates from different completer, so you use it as a completer but you're actually receiving results from more than one. Maybe it doesn't have the best name but it is a Completer for sure.

Comments from the review on Reviewable.io

ycmd/completers/completer.py, line 197 [r1] (raw file):
I see but the issue is that if you remove the inheritance, it works the same so what's the point to inherit?

Comments from the review on Reviewable.io

Review status: all files reviewed at latest revision, 2 unresolved discussions.

ycmd/completers/completer.py, line 197 [r1] (raw file):
The reason that even if you don't inherit it continue to work is that we're working in a dynamic language as python and we define all the methods we call on this object; if we were working on a static type language we could not remove the inheritance (or better we would have to specify an interface). So the point to continue to derive from Completer is to be explicit about the interface we want to satisfy.

Comments from the review on Reviewable.io

This LGTM.

Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

LGTM too. @homu r+

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

📌 Commit f45c847 has been approved by puremourning

Reviewed 1 of 10 files at r1, 6 of 9 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

⚡ Test exempted - status

This is really good stuff. Thanks for making the change.

Review status: all files reviewed at latest revision, 1 unresolved discussion.

ycmd/completers/completer.py, line 197 [r1] (raw file):
I agree with @vheon here; I prefer explicit interfaces over implicit ones. And like he said, GeneralCompleterStore is more than a container.

But @micbou has a good point in that there might be room for a SemanticCompleter class.

Comments from the review on Reviewable.io